Skip to content

[BC-breaking] Align ssim with psnr, metric output is tensor#2794

Merged
vfdev-5 merged 1 commit intopytorch:masterfrom
crj1998:fix_ssim
Dec 8, 2022
Merged

[BC-breaking] Align ssim with psnr, metric output is tensor#2794
vfdev-5 merged 1 commit intopytorch:masterfrom
crj1998:fix_ssim

Conversation

@crj1998
Copy link
Contributor

@crj1998 crj1998 commented Dec 7, 2022

Description:
For the current version, the output of psnr and ssim are differnt type, where the type of psnr.comute() is torch.Tensor but the ssim.compute() is float

psnr = PSNR(data_range=1.0)
psnr.update((torch.rand([4, 3, 16, 16]),torch.rand([4, 3, 16, 16])))
psnr = psnr.compute()
print(psnr, type(psnr))

ssim = SSIM(data_range=1.0, gaussian=True, sigma=1.5)
ssim.update((torch.rand([4, 3, 16, 16]), torch.rand([4, 3, 16, 16])))
ssim = ssim.compute()

print(ssim, type(ssim))

This is because in the ssim.py, there used .item()

ignite/ignite/metrics/psnr.py

Lines 120 to 124 in cdf7b47

@sync_all_reduce("_sum_of_batchwise_psnr", "_num_examples")
def compute(self) -> torch.Tensor:
if self._num_examples == 0:
raise NotComputableError("PSNR must have at least one example before it can be computed.")
return self._sum_of_batchwise_psnr / self._num_examples

while

ignite/ignite/metrics/ssim.py

Lines 182 to 186 in cdf7b47

@sync_all_reduce("_sum_of_ssim", "_num_examples")
def compute(self) -> float:
if self._num_examples == 0:
raise NotComputableError("SSIM must have at least one example before it can be computed.")
return (self._sum_of_ssim / self._num_examples).item()

The two metrics PSNR and SSIM are always used together. So I think it's better for the same input to have the same type of output. This is intuitive.

Check list:
I dont modify any unit test.

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added the module: metrics Metrics module label Dec 7, 2022
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 7, 2022 •
edited
Loading

@crj1998 thanks a lot for the PR ! I wonder about the common output type: tensor or float
Currently we have :

  • PSNR -> tensor
  • SSIM -> float
    and this PR makes:
  • PSNR -> tensor
  • SSIM -> tensor
    and I wonder if it would be better to output both floats :
  • PSNR -> float
  • SSIM -> float

@ydcjeff do you remember if there was a reason to output float value for SSIM and tensor for PSNR ?

cc @sadra-barikbin what do you think ?

@crj1998
Copy link
Contributor Author

crj1998 commented Dec 7, 2022

@crj1998 thanks a lot for the PR ! I wonder about the common output type: tensor or float Currently we have :

  • PSNR -> tensor
  • SSIM -> float
    and this PR makes:
  • PSNR -> tensor
  • SSIM -> tensor
    and I wonder if it would be better to output both floats :
  • PSNR -> float
  • SSIM -> float

@ydcjeff do you remember if there was a reason to output float value for SSIM and tensor for PSNR ?

cc @sadra-barikbin what do you think ?

I think that since ignite is an extension of pytorch, it's best to return the torch.Tensor type , because converting torch.Tensor to float only require an item() operation.

vfdev-5
vfdev-5 approved these changes Dec 7, 2022
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @crj1998 !

@vfdev-5 vfdev-5 changed the title align ssim with psnr [BC-breaking] Align ssim with psnr, metric output is tensor Dec 7, 2022
@vfdev-5 vfdev-5 merged commit 582fb00 into pytorch:master Dec 8, 2022
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 8, 2022

@crj1998 I merged your PR however running GPU tests there are few related failures : https://app.circleci.com/pipelines/github/pytorch/ignite/2900/workflows/45b4d299-599c-47e7-b0f6-121b0e4b5988/jobs/8072/parallel-runs/0/steps/0-107

Can you please check and fix them ?

@crj1998
Copy link
Contributor Author

crj1998 commented Dec 8, 2022

@crj1998 I merged your PR however running GPU tests there are few related failures : https://app.circleci.com/pipelines/github/pytorch/ignite/2900/workflows/45b4d299-599c-47e7-b0f6-121b0e4b5988/jobs/8072/parallel-runs/0/steps/0-107

Can you please check and fix them ?

Yeah, as i said before, I do noting about unit test, so the previous test examples which return float will be failed. I will update unit test in a new pr.

@crj1998 crj1998 mentioned this pull request Dec 8, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vfdev-5 vfdev-5 vfdev-5 approved these changes

Assignees

No one assigned

Labels

module: metrics Metrics module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@crj1998 @vfdev-5